backport: Merge bitcoin/bitcoin#30291, 30397, 22729, 29633, 29459#7300
backport: Merge bitcoin/bitcoin#30291, 30397, 22729, 29633, 29459#7300vijaydasmp wants to merge 5 commits into
Conversation
|
ad06e68 test: write functional test results to csv (tdb3) Pull request description: Adds argument `--resultsfile` to test_runner.py. Enables functional test results to be written to a (csv) file for processing by other applications (or for historical archiving). Test name, status, and duration are written to the file provided with the argument. Since `test_runner.py` is being touched, also fixes a misspelling (linter warning). Can split into its own commit if desired. #### Notes - Total runtime of functional tests has seemed to have increased on my development machines over the past few months (more tests added, individual test runtime increase, etc.). Was interested in recording test runtime data over time to detect trends. Initially searched `doc/benchmarking.md`, existing PRs, and Issues, but didn't immediately see this type of capability or alternate solutions (please chime in if you know of one!). Thought it would be beneficial to add this capability to `test_runner` to facilitate this type of data analysis (and potentially other use cases) - Saw https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#benchmarking-with-perf, and this PR's higher level data seems complimentary. - Was on the fence as to whether to expand `print_results()` (i.e. take advantage of the same loop over `test_results`) or implement in a separate `write_results()` function. Decided on the latter for now, but interested in reviewers' thoughts. #### Example 1: all tests pass ``` $ test/functional/test_runner.py --resultsfile functional_test_results.csv --cachedir=/mnt/tmp/cache --tmpdir=/mnt/tmp feature_blocksdir wallet_startup feature_config_args mempool_accept Temporary test directory at /mnt/tmp/test_runner_₿_🏃_20240614_201625 Test results will be written to functional_test_results.csv ... $ cat functional_test_results.csv test,status,duration(seconds) feature_blocksdir.py,Passed,1 feature_config_args.py,Passed,29 mempool_accept.py,Passed,9 wallet_startup.py,Passed,2 ALL,Passed,29 ``` #### Example 2: one test failure ``` $ cat functional_test_results.csv test,status,duration(seconds) feature_blocksdir.py,Passed,1 feature_config_args.py,Passed,28 wallet_startup.py,Passed,2 mempool_accept.py,Failed,1 ALL,Failed,28 ``` ACKs for top commit: maflcko: re-ACK ad06e68 kevkevinpal: tACK [ad06e68](bitcoin@ad06e68) achow101: ACK ad06e68 rkrux: tACK [ad06e68](bitcoin@ad06e68) brunoerg: ACK ad06e68 marcofleon: Good idea, tested ACK ad06e68 Tree-SHA512: 561194406cc744905518aa5ac6850c07c4aaecdaf5d4d8b250671b6e90093d4fc458f050e8a85374e66359cc0e0eaceba5eb24092c55f0d8f349d744a32ef76c
…l/net.cpp e233ec0 refactor: Use designated initializer (Hodlinator) Pull request description: Block was recently touched (e2d1f84) and the codebase recently switched to C++20 which allows this to improve robustness. Follow-up suggested in bitcoin#29625 (comment) ACKs for top commit: maflcko: ACK e233ec0 Tree-SHA512: ce3a18f513421e923710a43c8f97db1badb7ff5c6bdbfd62d9543312d2225731db5c14bef16feb47c43b84fad4dc24485086634b680feba422d2b7b363e13fa6
…startup on bind failure bca346a net: require P2P binds to succeed (Vasil Dimov) af55253 net: report an error if unable to bind on the Tor listening addr:port (Vasil Dimov) 9a7e5f4 net: don't extra bind for Tor if binds are restricted (Vasil Dimov) Pull request description: Make it possible to disable the Tor binding on `127.0.0.1:8334` and stop startup if any P2P bind fails instead of "if all P2P binds fail". Fixes bitcoin#22726 Fixes bitcoin#22727 ACKs for top commit: achow101: ACK bca346a cbergqvist: ACK bca346a pinheadmz: ACK bca346a Tree-SHA512: fabef89a957191eea4f3e3b6109d2b8389f27ecc74440a920b0c10f31fff00a85bcfd1eb3c91826c7169c618f4de8a8d0a260e2caf40fd854f07ea9a980d8603
d0e6564 log: Remove error() reference (Fabian Jahr) Pull request description: Mini-followup to bitcoin#29236 that was just merged. Removes a reference to `error()` that was missed in a comment. ACKs for top commit: ryanofsky: Code review ACK d0e6564. Just dropped LogPrintf reference since last review stickies-v: ACK d0e6564 Empact: ACK bitcoin@d0e6564 Tree-SHA512: 8abe4895951013c2ceca9a57743aacabaf8af831d07eee9ae8372c121c16e88b7226f0e537200c3464792e19ac7e03b57ba0be31f43add8802753972b0aefc48
bf264e0 test: check_mempool_result negative feerate (kevkevin) Pull request description: Adds test coverage in `mempool_accept.py` to check if a negative `maxfeerate` is input into `check_mempool_result` Asserts "Amount out of range" error message and `-3` error code Motivated by this [comment](https://github.com/bitcoin/bitcoin/pull/29434/files#r1491112250) ACKs for top commit: maflcko: lgtm ACK bf264e0 brunoerg: nice, utACK bf264e0 davidgumberg: Looks great, ACK bitcoin@bf264e0 Tree-SHA512: 58931b774cc887c616f2fd91af3ee65cc5db55acd8e2875c76de448c80bd4e020b057c5f4f85556431377f0d0e7553771fb285d1ec20cf64f64ec92a47776b78
|
✅ Review complete (commit 314e0a3) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 314e0a3b81
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if extra_args is None: | ||
| extra_args = self.extra_args | ||
|
|
There was a problem hiding this comment.
Avoid mutating shared node args when auto-inserting binds
start() appends auto-generated -bind arguments directly into extra_args, and when extra_args is omitted this aliases self.extra_args (line 232). In the framework, add_nodes() commonly seeds node args with shared empty lists ([[]] * num_nodes), so the first node that starts can mutate the list used by other nodes; subsequent nodes then inherit the first node’s bind ports and may fail with port-collision startup errors. Copy extra_args before appending so each node gets an isolated argument list.
Useful? React with 👍 / 👎.
WalkthroughThis pull request refactors network bind configuration across the test framework and core initialization. It introduces automatic bind address setup for test nodes by adding a Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/functional/mempool_accept.py (1)
80-86: 💤 Low valueClarify or remove the incomplete comment.
Line 86 contains the comment
# ... 0.99 passes, but the subsequent test case (lines 87-90) doesn't actually pass amaxfeerateparameter to demonstrate this. The comment may confuse readers who expect to see a positive feerate test immediately following the negative feerate test.Consider either:
- Removing the comment
- Expanding it to clarify that any positive value would pass (without needing explicit demonstration here)
- Adding an explicit test with a positive maxfeerate value
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/functional/mempool_accept.py` around lines 80 - 86, The inline comment "# ... 0.99 passes" is misleading because no positive maxfeerate test follows; update the test around the assert_raises_rpc_error call that uses check_mempool_result so the comment matches reality: either remove the comment, replace it with a clarifying note that positive maxfeerate values would pass without an explicit test, or add an explicit passing assertion that calls check_mempool_result with maxfeerate=0.99 (using the same raw_tx_in_block and expected successful result_expected) to demonstrate the positive-case; reference check_mempool_result and raw_tx_in_block when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/functional/test_framework/test_node.py`:
- Around line 94-98: The current logic only sets has_explicit_bind by checking
extra_conf for entries starting with "bind=", causing auto-injected binds to
ignore effective settings like listen, port, and whitebind; update the code in
the block that calls append_config(datadir, extra_conf) so it computes the
effective configuration after appending and derives has_explicit_bind from that
effective config: consider any explicit "bind=" or "-bind=" entries, any
whitebind entries, whether listen=0 is set, and whether a custom port is set
(i.e., port != p2p_port(self.index)); if any of those conditions apply, treat
the node as having an explicit bind and do not inject default
p2p_port(self.index). Use the existing symbols append_config, extra_conf,
has_explicit_bind, p2p_port(self.index), listen, port, whitebind to find and
change the logic.
In `@test/functional/test_runner.py`:
- Line 490: Replace the bare assertion on results_filepath.parent.exists() with
explicit error handling: check if not results_filepath.parent.exists() and raise
a clear exception (e.g., ValueError or FileNotFoundError) or call the same
error-reporting helper used elsewhere in this module, using the same message
"Results file parent directory does not exist" so the check on
results_filepath.parent is enforced even when Python is run with -O; update the
code near the results_filepath usage (the results_filepath variable check) to
follow the existing pattern used around the other validations in this file.
---
Nitpick comments:
In `@test/functional/mempool_accept.py`:
- Around line 80-86: The inline comment "# ... 0.99 passes" is misleading
because no positive maxfeerate test follows; update the test around the
assert_raises_rpc_error call that uses check_mempool_result so the comment
matches reality: either remove the comment, replace it with a clarifying note
that positive maxfeerate values would pass without an explicit test, or add an
explicit passing assertion that calls check_mempool_result with maxfeerate=0.99
(using the same raw_tx_in_block and expected successful result_expected) to
demonstrate the positive-case; reference check_mempool_result and
raw_tx_in_block when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ea4c1c31-e458-42d1-b146-76c59c425be7
📒 Files selected for processing (10)
src/init.cppsrc/logging.hsrc/net.cppsrc/test/util/net.cpptest/functional/feature_bind_extra.pytest/functional/mempool_accept.pytest/functional/test-shell.mdtest/functional/test_framework/test_node.pytest/functional/test_framework/util.pytest/functional/test_runner.py
| self.has_explicit_bind = False | ||
| if extra_conf is not None: | ||
| append_config(datadir, extra_conf) | ||
| # Remember if there is bind=... in the config file. | ||
| self.has_explicit_bind = any(e.startswith("bind=") for e in extra_conf) |
There was a problem hiding this comment.
Auto-injected binds ignore effective listen, port, and whitebind settings.
This only treats bind= / -bind= as explicit and always injects p2p_port(self.index). A node started with listen=0 in extra_conf, whitebind=..., or a custom port= will still get these default -bind= arguments, which can either fail init (-listen=0 + -bind) or make the node listen on the wrong port. Please derive the auto-bind decision from the full effective config before appending defaults.
Also applies to: 234-243
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/functional/test_framework/test_node.py` around lines 94 - 98, The
current logic only sets has_explicit_bind by checking extra_conf for entries
starting with "bind=", causing auto-injected binds to ignore effective settings
like listen, port, and whitebind; update the code in the block that calls
append_config(datadir, extra_conf) so it computes the effective configuration
after appending and derives has_explicit_bind from that effective config:
consider any explicit "bind=" or "-bind=" entries, any whitebind entries,
whether listen=0 is set, and whether a custom port is set (i.e., port !=
p2p_port(self.index)); if any of those conditions apply, treat the node as
having an explicit bind and do not inject default p2p_port(self.index). Use the
existing symbols append_config, extra_conf, has_explicit_bind,
p2p_port(self.index), listen, port, whitebind to find and change the logic.
| if args.resultsfile: | ||
| results_filepath = pathlib.Path(args.resultsfile) | ||
| # Stop early if the parent directory doesn't exist | ||
| assert results_filepath.parent.exists(), "Results file parent directory does not exist" |
There was a problem hiding this comment.
Replace assertion with explicit error handling for consistency.
Using assert for user input validation is inconsistent with the error handling pattern used elsewhere in this file (e.g., lines 497-498, 546-547), and assertions can be disabled with python -O.
Suggested fix
- assert results_filepath.parent.exists(), "Results file parent directory does not exist"
+ if not results_filepath.parent.exists():
+ print("ERROR: Results file parent directory does not exist")
+ sys.exit(1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert results_filepath.parent.exists(), "Results file parent directory does not exist" | |
| if not results_filepath.parent.exists(): | |
| print("ERROR: Results file parent directory does not exist") | |
| sys.exit(1) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/functional/test_runner.py` at line 490, Replace the bare assertion on
results_filepath.parent.exists() with explicit error handling: check if not
results_filepath.parent.exists() and raise a clear exception (e.g., ValueError
or FileNotFoundError) or call the same error-reporting helper used elsewhere in
this module, using the same message "Results file parent directory does not
exist" so the check on results_filepath.parent is enforced even when Python is
run with -O; update the code near the results_filepath usage (the
results_filepath variable check) to follow the existing pattern used around the
other validations in this file.
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Five-PR Bitcoin Core backport (bitcoin#30291, bitcoin#30397, bitcoin#22729, bitcoin#29633, bitcoin#29459) that merges cleanly against Dash's existing extensions in src/net.cpp and src/init.cpp. The behavioral change (bitcoin#22729: fail-fast on bind failures plus per-node bind injection in the test framework) is correctly carried over. One real but latent footgun in the test framework's bind injection (shared list mutation) plus two cosmetic comment issues. No blocking issues identified — Codex's 'blocking' framing of the shared-list bug is overstated because every current call site either short-circuits via has_explicit_bind or passes its own non-shared extra_args.
Reviewed commit: 314e0a3
🟡 1 suggestion(s) | 💬 2 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `test/functional/test_framework/test_node.py`:
- [SUGGESTION] lines 231-243: start() mutates the caller's extra_args list — latent shared-reference footgun
`start()` calls `extra_args.append(...)` directly on the list reference assigned from `self.extra_args` (or the caller-supplied list). In `BitcoinTestFramework.add_nodes`, the default path is `extra_args = [[]] * num_nodes`, which produces N references to the same empty list, all of which are stored as each node's `self.extra_args`.
In the current tree this does not blow up: with the default `bind_to_localhost_only=True`, `extra_conf` contains `bind=127.0.0.1`, so `self.has_explicit_bind` is True in `__init__` and the new injection block is skipped. The five tests that set `bind_to_localhost_only=False` (feature_discover, feature_bind_extra, feature_bind_port_discover, feature_bind_port_externalip, rpc_bind) either run a single node or supply per-node `extra_args` lists that already contain `-bind=...` (so the `any(e.startswith('-bind=')...)` short-circuit kicks in). So no test currently mutates a shared list.
But any future test that sets `bind_to_localhost_only=False` while relying on the default `extra_args` would silently get every node binding to node 0's `p2p_port`/`tor_port`, producing confusing port-collision failures. Copy the list before mutating to make the new injection block defensive against the shared-default.
| if extra_args is None: | ||
| extra_args = self.extra_args | ||
|
|
||
| # If listening and no -bind is given, then bitcoind would bind P2P ports on | ||
| # 0.0.0.0:P and 127.0.0.1:18445 (for incoming Tor connections), where P is | ||
| # a unique port chosen by the test framework and configured as port=P in | ||
| # bitcoin.conf. To avoid collisions on 127.0.0.1:18445, change it to | ||
| # 127.0.0.1:tor_port(). | ||
| will_listen = all(e != "-nolisten" and e != "-listen=0" for e in extra_args) | ||
| has_explicit_bind = self.has_explicit_bind or any(e.startswith("-bind=") for e in extra_args) | ||
| if will_listen and not has_explicit_bind: | ||
| extra_args.append(f"-bind=0.0.0.0:{p2p_port(self.index)}") | ||
| extra_args.append(f"-bind=127.0.0.1:{tor_port(self.index)}=onion") |
There was a problem hiding this comment.
🟡 Suggestion: start() mutates the caller's extra_args list — latent shared-reference footgun
start() calls extra_args.append(...) directly on the list reference assigned from self.extra_args (or the caller-supplied list). In BitcoinTestFramework.add_nodes, the default path is extra_args = [[]] * num_nodes, which produces N references to the same empty list, all of which are stored as each node's self.extra_args.
In the current tree this does not blow up: with the default bind_to_localhost_only=True, extra_conf contains bind=127.0.0.1, so self.has_explicit_bind is True in __init__ and the new injection block is skipped. The five tests that set bind_to_localhost_only=False (feature_discover, feature_bind_extra, feature_bind_port_discover, feature_bind_port_externalip, rpc_bind) either run a single node or supply per-node extra_args lists that already contain -bind=... (so the any(e.startswith('-bind=')...) short-circuit kicks in). So no test currently mutates a shared list.
But any future test that sets bind_to_localhost_only=False while relying on the default extra_args would silently get every node binding to node 0's p2p_port/tor_port, producing confusing port-collision failures. Copy the list before mutating to make the new injection block defensive against the shared-default.
💡 Suggested change
| if extra_args is None: | |
| extra_args = self.extra_args | |
| # If listening and no -bind is given, then bitcoind would bind P2P ports on | |
| # 0.0.0.0:P and 127.0.0.1:18445 (for incoming Tor connections), where P is | |
| # a unique port chosen by the test framework and configured as port=P in | |
| # bitcoin.conf. To avoid collisions on 127.0.0.1:18445, change it to | |
| # 127.0.0.1:tor_port(). | |
| will_listen = all(e != "-nolisten" and e != "-listen=0" for e in extra_args) | |
| has_explicit_bind = self.has_explicit_bind or any(e.startswith("-bind=") for e in extra_args) | |
| if will_listen and not has_explicit_bind: | |
| extra_args.append(f"-bind=0.0.0.0:{p2p_port(self.index)}") | |
| extra_args.append(f"-bind=127.0.0.1:{tor_port(self.index)}=onion") | |
| if extra_args is None: | |
| extra_args = self.extra_args | |
| extra_args = list(extra_args) | |
| # If listening and no -bind is given, then bitcoind would bind P2P ports on | |
| # 0.0.0.0:P and 127.0.0.1:18445 (for incoming Tor connections), where P is | |
| # a unique port chosen by the test framework and configured as port=P in | |
| # bitcoin.conf. To avoid collisions on 127.0.0.1:18445, change it to | |
| # 127.0.0.1:tor_port(). | |
| will_listen = all(e != "-nolisten" and e != "-listen=0" for e in extra_args) | |
| has_explicit_bind = self.has_explicit_bind or any(e.startswith("-bind=") for e in extra_args) | |
| if will_listen and not has_explicit_bind: | |
| extra_args.append(f"-bind=0.0.0.0:{p2p_port(self.index)}") | |
| extra_args.append(f"-bind=127.0.0.1:{tor_port(self.index)}=onion") |
source: ['claude', 'codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `test/functional/test_framework/test_node.py`:
- [SUGGESTION] lines 231-243: start() mutates the caller's extra_args list — latent shared-reference footgun
`start()` calls `extra_args.append(...)` directly on the list reference assigned from `self.extra_args` (or the caller-supplied list). In `BitcoinTestFramework.add_nodes`, the default path is `extra_args = [[]] * num_nodes`, which produces N references to the same empty list, all of which are stored as each node's `self.extra_args`.
In the current tree this does not blow up: with the default `bind_to_localhost_only=True`, `extra_conf` contains `bind=127.0.0.1`, so `self.has_explicit_bind` is True in `__init__` and the new injection block is skipped. The five tests that set `bind_to_localhost_only=False` (feature_discover, feature_bind_extra, feature_bind_port_discover, feature_bind_port_externalip, rpc_bind) either run a single node or supply per-node `extra_args` lists that already contain `-bind=...` (so the `any(e.startswith('-bind=')...)` short-circuit kicks in). So no test currently mutates a shared list.
But any future test that sets `bind_to_localhost_only=False` while relying on the default `extra_args` would silently get every node binding to node 0's `p2p_port`/`tor_port`, producing confusing port-collision failures. Copy the list before mutating to make the new injection block defensive against the shared-default.
| # If listening and no -bind is given, then bitcoind would bind P2P ports on | ||
| # 0.0.0.0:P and 127.0.0.1:18445 (for incoming Tor connections), where P is | ||
| # a unique port chosen by the test framework and configured as port=P in | ||
| # bitcoin.conf. To avoid collisions on 127.0.0.1:18445, change it to | ||
| # 127.0.0.1:tor_port(). |
There was a problem hiding this comment.
💬 Nitpick: Stale Bitcoin-specific port (18445) in carried-over comment
The comment references 127.0.0.1:18445, which is Bitcoin Core's regtest OnionServiceTargetPort. Dash's OnionServiceTargetPort values are different per network (9996 mainnet, 19996 testnet, 19796 devnet, 19896 regtest — see src/chainparamsbase.cpp). The code itself uses tor_port(self.index) so functionality is unaffected, but the explanatory example is misleading for Dash readers. Replace 18445 with a Dash-appropriate value or word it generically.
source: ['claude']
| rawtxs=[raw_tx_in_block], | ||
| maxfeerate=-0.01, | ||
| )) | ||
| # ... 0.99 passes |
There was a problem hiding this comment.
💬 Nitpick: Orphaned '# ... 0.99 passes' comment from bitcoin#29459 cherry-pick
Upstream bitcoin#29459 inserts the new negative-feerate assertion immediately before a pre-existing self.check_mempool_result(..., maxfeerate=0.99) call, where the trailing # ... 0.99 passes comment naturally describes the following line. Dash's mempool_accept.py never carried the maxfeerate=1 fails / 0.99 passes pair (pre-existing divergence), so the cherry-pick leaves the comment dangling above a check_mempool_result call that has no maxfeerate=0.99. Drop the comment or replace it with one that matches what the next call actually does. Cosmetic only.
source: ['claude']
bitcoin backports